From: tsteven4 Date: Sat, 9 Feb 2019 22:36:23 +0000 (-0700) Subject: bug fixes for xcsv reader. (#295) X-Git-Tag: archive/raspbian/1.10.0+ds-2+rpi1~1^2~12^2~8^2~49 X-Git-Url: https://dgit.raspbian.org/%22http:/www.example.com/cgi/%22https://%22Program/%22http:/www.example.com/cgi/%22https:/%22Program?a=commitdiff_plain;h=8f0e0f2a4788cf98e028f097409840ec20cd70ed;p=gpsbabel.git bug fixes for xcsv reader. (#295) For some lines data must be accumulated from several fields in arbitrary field order. Previously this was done with global variables. Now a variable to hold the accumulated data for a line is constructed/destructed for each line. This eliminates the possiblity of unintended communication between lines. This also eliminated some unintended communication between the reader and the writer through global variables that they previously shared (csv_track, csv_route). An ordering sensitivty to TRACK_NEW and TRACK_NAME is removed. For the reader, conditional allocation of route_heads is removed from xcsv_parse_val, and centralized in xcsv_data_read. This eliminates some undetected memory leaks which hid as "still reachable" due to the pointer being saved in a global variable (csv_track, csv_route). Undesirable reading of the route_head->Q variable is eliminated improving encapsulation. I beleive the intent was never to decide if the csv_track->Q was empty, i.e. it hadn't been added to the global track_head yet. I beleive what was desired was to decide if csv_track had any waypoints. In any event the centralization of route_head allocation and head addition makes new track handling much more straight forward and robust. The xcsv test case is enhanced to increase coverage. --- diff --git a/csv_util.cc b/csv_util.cc index af959182d..202dffe36 100644 --- a/csv_util.cc +++ b/csv_util.cc @@ -167,11 +167,19 @@ static double oldlat = 999; static int waypt_out_count; static route_head* csv_track, *csv_route; -static double utm_northing, utm_easting, utm_zone = 0; -static char utm_zonec; -static UrlLink* link_; -static gpsbabel_optional::optional lat_dir_positive; -static gpsbabel_optional::optional lon_dir_positive; + +struct xcsv_parse_data { + QString rte_name; + QString trk_name; + bool new_track{false}; + double utm_northing{0}; + double utm_easting{0}; + double utm_zone{0}; + char utm_zonec{'N'}; + UrlLink* link_{nullptr}; + gpsbabel_optional::optional lat_dir_positive; + gpsbabel_optional::optional lon_dir_positive; +}; #endif // CSVFMTS_ENABLED @@ -957,9 +965,9 @@ gmsd_init(Waypoint* wpt) /* xcsv_parse_val() - parse incoming data into the waypt structure. */ /* usage: xcsv_parse_val("-123.34", *waypt, *field_map) */ /*****************************************************************************/ -static void +static void xcsv_parse_val(const char* s, Waypoint* wpt, const field_map& fmp, - route_head** trk, const int line_no) + xcsv_parse_data* parse_data, const int line_no) { const char* enclosure = ""; geocache_data* gc_data = nullptr; @@ -994,16 +1002,16 @@ xcsv_parse_val(const char* s, Waypoint* wpt, const field_map& fmp, wpt->notes = csv_stringtrim(s, ""); break; case XT_URL: - if (!link_) { - link_ = new UrlLink; + if (!parse_data->link_) { + parse_data->link_ = new UrlLink; } - link_->url_ = QString(s).trimmed(); + parse_data->link_->url_ = QString(s).trimmed(); break; case XT_URL_LINK_TEXT: - if (!link_) { - link_ = new UrlLink; + if (!parse_data->link_) { + parse_data->link_ = new UrlLink; } - link_->url_link_text_ = QString(s).trimmed(); + parse_data->link_->url_link_text_ = QString(s).trimmed(); break; case XT_ICON_DESCR: wpt->icon_descr = QString(s).trimmed(); @@ -1065,9 +1073,9 @@ xcsv_parse_val(const char* s, Waypoint* wpt, const field_map& fmp, case XT_LAT_DIR: /* latitude N/S. */ if (*s == 'n' || *s == 'N') { - lat_dir_positive = true; + parse_data->lat_dir_positive = true; } else if (*s == 's' || *s == 'S') { - lat_dir_positive = false; + parse_data->lat_dir_positive = false; } else { warning("parse of string '%s' on line number %d as LAT_DIR failed. Expected 'n', 'N', 's' or 'S'.\n", s, line_no); } @@ -1075,9 +1083,9 @@ xcsv_parse_val(const char* s, Waypoint* wpt, const field_map& fmp, case XT_LON_DIR: /* longitude E/W. */ if (*s == 'e' || *s == 'E') { - lon_dir_positive = true; + parse_data->lon_dir_positive = true; } else if (*s == 'w' || *s == 'W') { - lon_dir_positive = false; + parse_data->lon_dir_positive = false; } else { warning("parse of string '%s' on line number %d as LON_DIR failed. Expected 'e', 'E', 'w' or 'W'.\n", s, line_no); } @@ -1088,33 +1096,33 @@ xcsv_parse_val(const char* s, Waypoint* wpt, const field_map& fmp, &wpt->latitude, &wpt->longitude, MYNAME); break; case XT_UTM_ZONE: - utm_zone = atoi(s); + parse_data->utm_zone = atoi(s); break; case XT_UTM_ZONEC: - utm_zonec = s[0]; + parse_data->utm_zonec = s[0]; break; case XT_UTM_ZONEF: - utm_zone = atoi(s); - utm_zonec = s[strlen(s) - 1]; + parse_data->utm_zone = atoi(s); + parse_data->utm_zonec = s[strlen(s) - 1]; break; case XT_UTM_EASTING: - utm_easting = atof(s); + parse_data->utm_easting = atof(s); break; case XT_UTM_NORTHING: - utm_northing = atof(s); + parse_data->utm_northing = atof(s); break; case XT_UTM: { char* ss; int i = 0; - utm_zone = strtod(s, &ss); - utm_zonec = ss[i]; + parse_data->utm_zone = strtod(s, &ss); + parse_data->utm_zonec = ss[i]; ss++; - utm_easting = strtod(ss, &ss); + parse_data->utm_easting = strtod(ss, &ss); while (*ss && !isdigit(*ss)) { ss++; } - utm_northing = strtod(ss, nullptr); + parse_data->utm_northing = strtod(ss, nullptr); } break; /* ALTITUDE CONVERSIONS ************************************************/ @@ -1296,23 +1304,13 @@ xcsv_parse_val(const char* s, Waypoint* wpt, const field_map& fmp, break; /* Tracks and routes *********************************************/ case XT_ROUTE_NAME: - if (csv_route) { - csv_route->rte_name = csv_stringtrim(s, enclosure); - } + parse_data->rte_name = csv_stringtrim(s, enclosure); break; case XT_TRACK_NEW: - if (atoi(s) && csv_track && !QUEUE_EMPTY(&csv_track->Q)) { - *trk = route_head_alloc(); - csv_track = *trk; - - track_add_head(*trk); - } + parse_data->new_track = atoi(s); break; case XT_TRACK_NAME: - if (!csv_track) { - csv_track = route_head_alloc(); - } - csv_track->rte_name = csv_stringtrim(s, enclosure); + parse_data->trk_name = csv_stringtrim(s, enclosure); break; /* OTHER STUFF ***************************************************/ @@ -1402,17 +1400,6 @@ xcsv_data_read() int linecount = 0; route_head* rte = nullptr; route_head* trk = nullptr; - utm_northing = 0; - utm_easting = 0; - utm_zone = 0; - utm_zonec = 'N'; - - csv_route = csv_track = nullptr; - if (xcsv_file.datatype == trkdata) { - csv_track = trk; - } else if (xcsv_file.datatype == rtedata) { - csv_route = rte; - } while (true) { QString buff = xcsv_file.stream->readLine(); @@ -1445,6 +1432,8 @@ xcsv_data_read() } if (!buff.isEmpty()) { Waypoint* wpt_tmp = new Waypoint; + // initialize parse data for accumulation of line results from all fields in this line. + xcsv_parse_data parse_data; // tbuf is a temporary copy of buff since we modify it. :-( char *tbuf = xstrdup(buff); const char* s = tbuf; @@ -1455,8 +1444,6 @@ xcsv_data_read() fatal(MYNAME ": attempt to read, but style '%s' has no IFIELDs in it.\n", CSTR(xcsv_file.description)? CSTR(xcsv_file.description) : "unknown"); } - lat_dir_positive.reset(); - lon_dir_positive.reset(); int ifield_idx = 0; /* now rip the line apart, advancing the queue for each tear @@ -1464,7 +1451,7 @@ xcsv_data_read() */ while (s) { const field_map& fmp = xcsv_file.ifields.at(ifield_idx++); - xcsv_parse_val(s, wpt_tmp, fmp, &trk, linecount); + xcsv_parse_val(s, wpt_tmp, fmp, &parse_data, linecount); if (ifield_idx >= xcsv_file.ifields.size()) { /* we've wrapped the queue. so stop parsing! */ @@ -1480,10 +1467,10 @@ xcsv_data_read() // If XT_LAT_DIR(XT_LON_DIR) was an input field, and the latitude(longitude) is positive, // assume the latitude(longitude) was the absolute value and take the sign from XT_LAT_DIR(XT_LON_DIR). - if (lat_dir_positive.has_value() && !lat_dir_positive.value() && (wpt_tmp->latitude > 0.0)) { + if (parse_data.lat_dir_positive.has_value() && !parse_data.lat_dir_positive.value() && (wpt_tmp->latitude > 0.0)) { wpt_tmp->latitude = -wpt_tmp->latitude; } - if (lon_dir_positive.has_value() && !lon_dir_positive.value() && (wpt_tmp->longitude > 0.0)) { + if (parse_data.lon_dir_positive.has_value() && !parse_data.lon_dir_positive.value() && (wpt_tmp->longitude > 0.0)) { wpt_tmp->longitude = -wpt_tmp->longitude; } @@ -1493,18 +1480,18 @@ xcsv_data_read() &wpt_tmp->latitude, &wpt_tmp->longitude, &alt, xcsv_file.gps_datum); } - if (utm_easting || utm_northing) { + if (parse_data.utm_easting || parse_data.utm_northing) { GPS_Math_UTM_EN_To_Known_Datum(&wpt_tmp->latitude, &wpt_tmp->longitude, - utm_easting, utm_northing, - utm_zone, utm_zonec, + parse_data.utm_easting, parse_data.utm_northing, + parse_data.utm_zone, parse_data.utm_zonec, DATUM_WGS84); } - if (link_) { - wpt_tmp->AddUrlLink(*link_); - delete link_; - link_ = nullptr; + if (parse_data.link_) { + wpt_tmp->AddUrlLink(*parse_data.link_); + delete parse_data.link_; + parse_data.link_ = nullptr; } switch (xcsv_file.datatype) { @@ -1513,19 +1500,23 @@ xcsv_data_read() waypt_add(wpt_tmp); break; case trkdata: - if (trk == nullptr) { + if ((trk == nullptr) || parse_data.new_track) { trk = route_head_alloc(); - csv_track = trk; track_add_head(trk); } + if (!parse_data.trk_name.isEmpty()) { + trk->rte_name = parse_data.trk_name; + } track_add_wpt(trk, wpt_tmp); break; case rtedata: if (rte == nullptr) { rte = route_head_alloc(); - csv_route = rte; route_add_head(rte); } + if (!parse_data.rte_name.isEmpty()) { + rte->rte_name = parse_data.rte_name; + } route_add_wpt(rte, wpt_tmp); break; default: diff --git a/reference/route/route1.csv b/reference/route/route1.csv new file mode 100644 index 000000000..4d3ae18a7 --- /dev/null +++ b/reference/route/route1.csv @@ -0,0 +1,6 @@ +40.0,-104.0,myroute +40.1,-104.1,myroute +41.0,-105.0,myroute +41.1,-105.1,myroute +41.2,-105.2,myroute +42.0,-106.0,myroute diff --git a/reference/route/route1~csv.gpx b/reference/route/route1~csv.gpx new file mode 100644 index 000000000..23c5f0b65 --- /dev/null +++ b/reference/route/route1~csv.gpx @@ -0,0 +1,26 @@ + + + + + + myroute + + RPT001 + + + RPT002 + + + RPT003 + + + RPT004 + + + RPT005 + + + RPT006 + + + diff --git a/reference/track/track1-2~csv.gpx b/reference/track/track1-2~csv.gpx new file mode 100644 index 000000000..cacf14d9d --- /dev/null +++ b/reference/track/track1-2~csv.gpx @@ -0,0 +1,26 @@ + + + + + + mytrack-0 + + + + + + + mytrack-1 + + + + + + + + mytrack-2 + + + + + diff --git a/reference/track/track1.csv b/reference/track/track1.csv new file mode 100644 index 000000000..0652e23f1 --- /dev/null +++ b/reference/track/track1.csv @@ -0,0 +1,6 @@ +40.0,-104.0,mytrack-0,1 +40.1,-104.1,mytrack-0,0 +41.0,-105.0,mytrack-1,1 +41.1,-105.1,mytrack-1,0 +41.2,-105.2,mytrack-1,0 +42.0,-106.0,mytrack-2,1 diff --git a/reference/track/track2.csv b/reference/track/track2.csv new file mode 100644 index 000000000..450848aff --- /dev/null +++ b/reference/track/track2.csv @@ -0,0 +1,6 @@ +40.0,-104.0,1,mytrack-0 +40.1,-104.1,0,mytrack-0 +41.0,-105.0,1,mytrack-1 +41.1,-105.1,0,mytrack-1 +41.2,-105.2,0,mytrack-1 +42.0,-106.0,1,mytrack-2 diff --git a/testo.d/xcsv.test b/testo.d/xcsv.test index e78dba342..5cbfa297d 100644 --- a/testo.d/xcsv.test +++ b/testo.d/xcsv.test @@ -32,3 +32,41 @@ echo "IFIELD HMSG_TIME, , %d:%d:%d" >> ${TMPDIR}/testo2.style rm -f ${TMPDIR}/grid-utm~xscv.gpx gpsbabel -i xcsv,style=${TMPDIR}/testo2.style -f ${REFERENCE}/grid-utm.csv -o gpx -F ${TMPDIR}/grid-utm~xscv.gpx compare ${REFERENCE}/grid-utm~xscv.gpx ${TMPDIR}/grid-utm~xscv.gpx + +# test TRACK_NAME, TRACK_NEW +echo 'DESCRIPTION track style test 1' >>${TMPDIR}/track1.style +echo 'EXTENSION csv' >>${TMPDIR}/track1.style +echo 'FIELD_DELIMITER COMMA' >>${TMPDIR}/track1.style +echo 'RECORD_DELIMITER NEWLINE' >>${TMPDIR}/track1.style +echo 'DATATYPE TRACK' >>${TMPDIR}/track1.style +echo 'IFIELD LON_DECIMAL,"","%f"' >>${TMPDIR}/track1.style +echo 'IFIELD LAT_DECIMAL,"","%f"' >>${TMPDIR}/track1.style +echo 'IFIELD TRACK_NAME,"","%s"' >>${TMPDIR}/track1.style +echo 'IFIELD TRACK_NEW,"","%d"' >>${TMPDIR}/track1.style +gpsbabel -i xcsv,style=${TMPDIR}/track1.style -f ${REFERENCE}/track/track1.csv -o gpx -F ${TMPDIR}/track1~csv.gpx +compare ${REFERENCE}/track/track1-2~csv.gpx ${TMPDIR}/track1~csv.gpx + +# flip TRACK_NAME, TRACK_NEW order +echo 'DESCRIPTION track style test 2' >>${TMPDIR}/track2.style +echo 'EXTENSION csv' >>${TMPDIR}/track2.style +echo 'FIELD_DELIMITER COMMA' >>${TMPDIR}/track2.style +echo 'RECORD_DELIMITER NEWLINE' >>${TMPDIR}/track2.style +echo 'DATATYPE TRACK' >>${TMPDIR}/track2.style +echo 'IFIELD LON_DECIMAL,"","%f"' >>${TMPDIR}/track2.style +echo 'IFIELD LAT_DECIMAL,"","%f"' >>${TMPDIR}/track2.style +echo 'IFIELD TRACK_NEW,"","%d"' >>${TMPDIR}/track2.style +echo 'IFIELD TRACK_NAME,"","%s"' >>${TMPDIR}/track2.style +gpsbabel -i xcsv,style=${TMPDIR}/track2.style -f ${REFERENCE}/track/track2.csv -o gpx -F ${TMPDIR}/track2~csv.gpx +compare ${REFERENCE}/track/track1-2~csv.gpx ${TMPDIR}/track2~csv.gpx + +# ROUTE_NAME +echo 'DESCRIPTION route style test 1' >>${TMPDIR}/route1.style +echo 'EXTENSION csv' >>${TMPDIR}/route1.style +echo 'FIELD_DELIMITER COMMA' >>${TMPDIR}/route1.style +echo 'RECORD_DELIMITER NEWLINE' >>${TMPDIR}/route1.style +echo 'DATATYPE ROUTE' >>${TMPDIR}/route1.style +echo 'IFIELD LON_DECIMAL,"","%f"' >>${TMPDIR}/route1.style +echo 'IFIELD LAT_DECIMAL,"","%f"' >>${TMPDIR}/route1.style +echo 'IFIELD ROUTE_NAME,"","%s"' >>${TMPDIR}/route1.style +gpsbabel -i xcsv,style=${TMPDIR}/route1.style -f ${REFERENCE}/route/route1.csv -o gpx -F ${TMPDIR}/route1~csv.gpx +compare ${REFERENCE}/route/route1~csv.gpx ${TMPDIR}/route1~csv.gpx